Add interval array #60
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
=======================================
Coverage ? 82.52%
=======================================
Files ? 38
Lines ? 1774
Branches ? 0
=======================================
Hits ? 1464
Misses ? 310
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Alex-PLACET
force-pushed
the
add_interval_array
branch
from
a5d93e6 to
1b108d4
Compare
Alex-PLACET
requested review from
Hind-M and
JohanMabille
and removed request for
Hind-M and
JohanMabille
| namespace sparrow_ipc | ||
| { | ||
| template <typename T> | ||
| [[nodiscard]] sparrow::interval_array<T> deserialize_non_owning_interval_array( |
There was a problem hiding this comment.
It seems this function is exactly the same (except from data type) as the one used in primitive_array, so we should probably refactor and use some generic function.
There was a problem hiding this comment.
I addressed your point, now there is a: deserialize_non_owning_simple_array
| const std::string name = field->name() == nullptr ? "" : field->name()->str(); | ||
| const bool nullable = field->nullable(); | ||
| const auto field_type = field->type_type(); | ||
| // TODO rename all the deserialize_non_owning... fcts since this is not correct anymore |
There was a problem hiding this comment.
Why remove this? This is actually still relevant as we are using now optionally_owned_buffer (which is not necessarily non owning as the functions names suggest).
|
Should this be linked to the corresponding issue? |
Alex-PLACET
force-pushed
the
add_interval_array
branch
from
483b536 to
5378771
Compare
| ${SPARROW_IPC_INCLUDE_DIR}/sparrow_ipc/config/sparrow_ipc_version.hpp | ||
| ${SPARROW_IPC_INCLUDE_DIR}/sparrow_ipc/compression.hpp | ||
| ${SPARROW_IPC_INCLUDE_DIR}/sparrow_ipc/deserialize_fixedsizebinary_array.hpp | ||
| ${SPARROW_IPC_INCLUDE_DIR}/sparrow_ipc/deserialize_interval_array.hpp |
There was a problem hiding this comment.
deserialize_array_impl.hpp is missing from the CMakeLists.txt.
| const std::string name = field->name() == nullptr ? "" : field->name()->str(); | ||
| const bool nullable = field->nullable(); | ||
| const auto field_type = field->type_type(); | ||
| // TODO rename all the deserialize_non_owning... fcts since this is not correct anymore |
There was a problem hiding this comment.
Can we add back the comment as it's still relevant?
| ArrowSchema schema = make_non_owning_arrow_schema( | ||
| format, | ||
| name.data(), | ||
| return sparrow_ipc::detail::deserialize_non_owning_simple_array<sparrow::primitive_array, T>( |
There was a problem hiding this comment.
| return sparrow_ipc::detail::deserialize_non_owning_simple_array<sparrow::primitive_array, T>( | |
| return detail::deserialize_non_owning_simple_array<sparrow::primitive_array, T>( |
Using sparrow_ipc:: is redundant as we are already in that namespace and not consistent with the rest of the codebase.
3 participants
No description provided.